-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First draft for a buffered writing of files #183
Conversation
- for now it's in testing phase, so I flush at the end of the simulation, it needs to be parametrised - adds ask methods with CharSequence in addition to String - need to add an operator to flush ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 1 findings(s) 🚩
- Improving Code Health: 1 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
- stronger typing of owners (switching to SimulationAgent instead of Object) - replacing OwnerWriteAsks class by a simple Map<SimulationAgent, StringBuilder> - adding enum for the buffering strategies - adding a different map to manage the cycle based buffering - removing unused OwnerWriteAsksQueue class - adding different methods to flush according to a given strategy - flushing is now strictly for an owner, before it would also flush the previous writes of different owners in a file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 1 findings(s) 🚩
- Improving Code Health: 4 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
- adds a parameter into the ISaveStatement - make the flush operation continue even if it fails on one file - cleanup a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 6 findings(s) 🚩
- Improving Code Health: 5 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
I've finished implementing the file buffering in the csv and text delegates for the save statement.
Here is an example model to showcase the different behaviors:
Here is what's left to do:
|
… + some improvements on rewrite cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Code Health Quality Gates: FAILED
- Declining Code Health: 7 findings(s) 🚩
- Improving Code Health: 5 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
- adds a SaveOptions class to carry all saving options - buffered writing now manage the charset - buffered writing now handles the rewrite facets - adds a WriteTask class to carry informations about encoding and rewriting in buffered writing - adds rewrite option in json writer (was in append mode by default)
I have a functional first version, @AlexisDrogoul could you have a look and tell me if you see anything wrong ? And here is a model to test the functionalities:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 3 findings(s) 🚩
- Improving Code Health: 20 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
@@ -77,24 +75,68 @@ public void save(final IScope scope, final IExpression item, final OutputStream | |||
* Signals that an I/O exception has occurred. | |||
*/ | |||
@Override | |||
public void save(final IScope scope, final IExpression item, final File file, final String code, | |||
final boolean addHeader, final String type, final Object attributesToSave) | |||
public void save(final IScope scope, final IExpression item, final File file, final SaveOptions saveOptions) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Getting better: Complex Method
save decreases in cyclomatic complexity from 24 to 20, threshold = 9
final IExpression bufferingStrategy = desc.getFacetExpr(IKeyword.BUFFERING); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ Getting worse: Complex Method
SaveStatement.SaveValidator.validate increases in cyclomatic complexity from 50 to 51, threshold = 9
@@ -58,18 +59,20 @@ public class ImageSaver extends AbstractSaver { | |||
* Signals that an I/O exception has occurred. | |||
*/ | |||
@Override | |||
public void save(final IScope scope, final IExpression item, final File file, final String code, | |||
final boolean addHeader, final String type, final Object attributesToSave) throws IOException { | |||
public void save(final IScope scope, final IExpression item, final File file, final SaveOptions options) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Getting worse: Complex Method
save increases in cyclomatic complexity from 12 to 13, threshold = 9
public SaveOptions(final String code, final boolean addHeader, final String type, final Object attributesToSave, | ||
BufferingStrategies bufferingStrategy, final boolean rewrite) { | ||
this.code = code; | ||
this.addHeader = addHeader; | ||
this.type = type; | ||
this.attributesToSave = attributesToSave; | ||
this.bufferingStrategy = bufferingStrategy; | ||
this.rewrite = rewrite; | ||
writeCharset = StandardCharsets.UTF_8; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Constructor Over-Injection
SaveOptions has 6 arguments, threshold = 5
- renames writecontroller to bufferingcontroller and writetask to bufferingtask - makes bufferingtask inner static - adds maps for buffered write statements in bufferingcontroller - adds base functions to handle buffered write statements in bufferingcontroller - some light refactoring - fixes documentation of flush_all_files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 3 findings(s) 🚩
- Improving Code Health: 20 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
} | ||
|
||
// If the last element of the list is not of the same color as the currently requested color we append a new task with the new color | ||
if (requests.size() == 0 || (requests.get(requests.size()-1).color != null && !requests.get(requests.size()-1).color.equals(color))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Complex Conditional
appendWriteConsoleRequestToMap has 1 complex conditionals with 2 branches, threshold = 2
…statement - moves some part of the buffering strategies code from savestatement to BufferingController. Should probably by moved to a separate class when refactoring - some refactoring/renaming - Adds the per_agent strategy in save and write statement. The data would be written when the calling agent is disposed. Need to check that this does not impact performances on simulations with many agents - append a Strings.LN in each buffering call. This should later be removed and done directly in the write statement - some formatting on Default Scheduler.gaml because why not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 4 findings(s) 🚩
- Improving Code Health: 20 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
…d of a line in the console
- minor fixes for spotbugs - fixes removing last characters in case of no buffering (forgotten code from the addition of end facet) - better names - documents some methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
- Declining Code Health: 3 findings(s) 🚩
- Improving Code Health: 20 findings(s) ✅
- Affected Hotspots: 1 files(s) 🔥
I'm merging this as it seems to be working and I opened separate issues for the remaining improvements I described above |
It's a work in progress, I'm currently testing it and not sure yet it's actually more performant.
I tested it with this model (thanks @ptaillandier) and for now it seems to be way better, but it needs more work to be sure: